[HUDI-3612] Should create new TypedProperties when modifying it#5027
[HUDI-3612] Should create new TypedProperties when modifying it#5027codope merged 6 commits intoapache:masterfrom
Conversation
...n/java/org/apache/hudi/client/clustering/run/strategy/SparkSortAndSizeExecutionStrategy.java
Show resolved
Hide resolved
| props.put(HoodieWriteConfig.AUTO_COMMIT_ENABLE.key(), Boolean.FALSE.toString()); | ||
| props.put(HoodieStorageConfig.PARQUET_MAX_FILE_SIZE.key(), String.valueOf(getWriteConfig().getClusteringTargetFileMaxBytes())); | ||
| HoodieWriteConfig newConfig = HoodieWriteConfig.newBuilder().withProps(props).build(); | ||
| getWriteConfig().setValue(HoodieWriteConfig.AUTO_COMMIT_ENABLE, Boolean.FALSE.toString()); |
There was a problem hiding this comment.
Isn't it possible this could cause the same issue, because getWriteConfig() is returning the same object? Maybe fix in superclass as well?
There was a problem hiding this comment.
As TypedProperties is thread-safe now, so changing this here should not cause the same issue.
Please correct me if I'm wrong, I thought the purpose of this pr is avoiding to set bulk related configures(bulkInsertParallelism, parquet max file size) to the original writeConfig(getWriteConfig).
There was a problem hiding this comment.
Yeah TypedProperties is thread-safe. But, why can't we do the same for this config like how you did for bulk related configures(bulkInsertParallelism, parquet max file size) i.e. set auto_commit_enable in newConfig on L64 below.
There was a problem hiding this comment.
Because disable auto_commit_enable will be checked in
Setting it in newConfig will not take effect as it uses BaseCommitActionExecutor.config to check(which is then same with getWriteConfig()).
Maybe I can set auto_commit_enable in BaseCommitActionExecutor after L236, WDYT?
|
Gentle ping @codope |
| props.put(HoodieWriteConfig.AUTO_COMMIT_ENABLE.key(), Boolean.FALSE.toString()); | ||
| props.put(HoodieStorageConfig.PARQUET_MAX_FILE_SIZE.key(), String.valueOf(getWriteConfig().getClusteringTargetFileMaxBytes())); | ||
| HoodieWriteConfig newConfig = HoodieWriteConfig.newBuilder().withProps(props).build(); | ||
| getWriteConfig().setValue(HoodieWriteConfig.AUTO_COMMIT_ENABLE, Boolean.FALSE.toString()); |
There was a problem hiding this comment.
Yeah TypedProperties is thread-safe. But, why can't we do the same for this config like how you did for bulk related configures(bulkInsertParallelism, parquet max file size) i.e. set auto_commit_enable in newConfig on L64 below.
|
@hudi-bot run azure |
|
@codope can merge this now |
|
Thanks @boneanxs for working on this. |
4 participants
Tips
What is the purpose of the pull request
(For example: This pull request adds quick-start document.)
Brief change log
(for example:)
Verify this pull request
(Please pick either of the following options)
This pull request is a trivial rework / code cleanup without any test coverage.
(or)
This pull request is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Committer checklist
Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.